UX: show complete URL path if website domain is same as instance domain #6
Conversation
WalkthroughThe changes remove the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server (Serializer)
participant Database
Client->>Server (Serializer): Request user profile data
Server (Serializer)->>Database: Fetch user record
Database-->>Server (Serializer): Return user data
Server (Serializer)->>Server (Serializer): Compute website_name from website URL and instance hostname
Server (Serializer)-->>Client: Return user profile JSON (includes website_name)
Client->>Client: Render user profile with model.website_name
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/assets/javascripts/discourse/controllers/user.js.es6(0 hunks)app/assets/javascripts/discourse/models/user.js.es6(1 hunks)app/assets/javascripts/discourse/templates/user/user.hbs(1 hunks)app/serializers/user_serializer.rb(2 hunks)spec/serializers/user_serializer_spec.rb(1 hunks)
💤 Files with no reviewable changes (1)
- app/assets/javascripts/discourse/controllers/user.js.es6
🧰 Additional context used
🧬 Code Graph Analysis (1)
spec/serializers/user_serializer_spec.rb (1)
app/serializers/user_serializer.rb (1)
website(133-135)
🪛 RuboCop (1.76.1)
app/serializers/user_serializer.rb
[convention] 137-151: Assignment Branch Condition size for website_name is too high. [<2, 35, 12> 37.05/23]
(Metrics/AbcSize)
[convention] 137-151: Cyclomatic complexity for website_name is too high. [8/7]
(Metrics/CyclomaticComplexity)
[convention] 137-151: Perceived complexity for website_name is too high. [9/8]
(Metrics/PerceivedComplexity)
[convention] 138-138: Avoid using rescue in its modifier form.
(Style/RescueModifier)
🔇 Additional comments (8)
app/serializers/user_serializer.rb (2)
43-43: LGTM! Clean attribute addition.The
website_nameattribute is properly added to the serializer attributes list.
153-155: LGTM! Proper conditional inclusion.The
include_website_namemethod correctly follows the established pattern for conditional attribute inclusion, ensuringwebsite_nameis only serialized when a website is present.app/assets/javascripts/discourse/models/user.js.es6 (1)
59-59: LGTM! Documentation fix.The JSDoc property tag is correctly updated to match the actual property name
profileBackground.app/assets/javascripts/discourse/templates/user/user.hbs (3)
66-66: LGTM! Correct template update.The conditional check is properly updated to use the new serialized attribute
model.website_name.
69-69: LGTM! Consistent attribute usage.The link text correctly uses
model.website_namewhile maintaining the properhrefattribute withmodel.website.
71-71: LGTM! Complete template consistency.The span content is properly updated to use
model.website_namefor display while keeping the title attribute asmodel.website.spec/serializers/user_serializer_spec.rb (2)
69-74: LGTM! Enhanced test setup.The website URL is updated to include a path component, providing more comprehensive test coverage for the website_name functionality.
76-91: Excellent test coverage for website_name logic.The test cases properly cover all three domain matching scenarios implemented in the serializer:
- Different domains → returns host only
- Same domains → returns host + path
- Subdomain relationship → returns host + path
The use of
Discourse.stubs(:current_hostname)correctly simulates different hostname scenarios.
| def website_name | ||
| website_host = URI(website.to_s).host rescue nil | ||
| discourse_host = Discourse.current_hostname | ||
| return if website_host.nil? | ||
| if website_host == discourse_host | ||
| # example.com == example.com | ||
| website_host + URI(website.to_s).path | ||
| elsif (website_host.split('.').length == discourse_host.split('.').length) && discourse_host.split('.').length > 2 | ||
| # www.example.com == forum.example.com | ||
| website_host.split('.')[1..-1].join('.') == discourse_host.split('.')[1..-1].join('.') ? website_host + URI(website.to_s).path : website_host | ||
| else | ||
| # example.com == forum.example.com | ||
| discourse_host.ends_with?("." << website_host) ? website_host + URI(website.to_s).path : website_host | ||
| end | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor to reduce complexity and improve readability.
The method has high cyclomatic complexity (8/7) and contains several style issues. Consider breaking this down into smaller, more focused methods.
def website_name
- website_host = URI(website.to_s).host rescue nil
- discourse_host = Discourse.current_hostname
- return if website_host.nil?
- if website_host == discourse_host
- # example.com == example.com
- website_host + URI(website.to_s).path
- elsif (website_host.split('.').length == discourse_host.split('.').length) && discourse_host.split('.').length > 2
- # www.example.com == forum.example.com
- website_host.split('.')[1..-1].join('.') == discourse_host.split('.')[1..-1].join('.') ? website_host + URI(website.to_s).path : website_host
- else
- # example.com == forum.example.com
- discourse_host.ends_with?("." << website_host) ? website_host + URI(website.to_s).path : website_host
- end
+ return unless website_host = extract_website_host
+
+ discourse_host = Discourse.current_hostname
+
+ if should_show_full_path?(website_host, discourse_host)
+ website_host + website_path
+ else
+ website_host
+ end
+end
+
+private
+
+def extract_website_host
+ begin
+ URI(website.to_s).host
+ rescue URI::InvalidURIError
+ nil
+ end
+end
+
+def website_path
+ begin
+ URI(website.to_s).path
+ rescue URI::InvalidURIError
+ ""
+ end
+end
+
+def should_show_full_path?(website_host, discourse_host)
+ exact_domain_match?(website_host, discourse_host) ||
+ subdomain_of_same_domain?(website_host, discourse_host) ||
+ parent_domain_match?(website_host, discourse_host)
+end
+
+def exact_domain_match?(website_host, discourse_host)
+ website_host == discourse_host
+end
+
+def subdomain_of_same_domain?(website_host, discourse_host)
+ return false unless both_have_subdomains?(website_host, discourse_host)
+
+ website_parts = website_host.split('.')
+ discourse_parts = discourse_host.split('.')
+
+ website_parts[1..-1] == discourse_parts[1..-1]
+end
+
+def parent_domain_match?(website_host, discourse_host)
+ discourse_host.ends_with?(".#{website_host}")
+end
+
+def both_have_subdomains?(website_host, discourse_host)
+ website_host.split('.').length == discourse_host.split('.').length &&
+ discourse_host.split('.').length > 2📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def website_name | |
| website_host = URI(website.to_s).host rescue nil | |
| discourse_host = Discourse.current_hostname | |
| return if website_host.nil? | |
| if website_host == discourse_host | |
| # example.com == example.com | |
| website_host + URI(website.to_s).path | |
| elsif (website_host.split('.').length == discourse_host.split('.').length) && discourse_host.split('.').length > 2 | |
| # www.example.com == forum.example.com | |
| website_host.split('.')[1..-1].join('.') == discourse_host.split('.')[1..-1].join('.') ? website_host + URI(website.to_s).path : website_host | |
| else | |
| # example.com == forum.example.com | |
| discourse_host.ends_with?("." << website_host) ? website_host + URI(website.to_s).path : website_host | |
| end | |
| end | |
| def website_name | |
| return unless website_host = extract_website_host | |
| discourse_host = Discourse.current_hostname | |
| if should_show_full_path?(website_host, discourse_host) | |
| website_host + website_path | |
| else | |
| website_host | |
| end | |
| end | |
| private | |
| def extract_website_host | |
| begin | |
| URI(website.to_s).host | |
| rescue URI::InvalidURIError | |
| nil | |
| end | |
| end | |
| def website_path | |
| begin | |
| URI(website.to_s).path | |
| rescue URI::InvalidURIError | |
| "" | |
| end | |
| end | |
| def should_show_full_path?(website_host, discourse_host) | |
| exact_domain_match?(website_host, discourse_host) || | |
| subdomain_of_same_domain?(website_host, discourse_host) || | |
| parent_domain_match?(website_host, discourse_host) | |
| end | |
| def exact_domain_match?(website_host, discourse_host) | |
| website_host == discourse_host | |
| end | |
| def subdomain_of_same_domain?(website_host, discourse_host) | |
| return false unless both_have_subdomains?(website_host, discourse_host) | |
| website_parts = website_host.split('.') | |
| discourse_parts = discourse_host.split('.') | |
| website_parts[1..-1] == discourse_parts[1..-1] | |
| end | |
| def parent_domain_match?(website_host, discourse_host) | |
| discourse_host.ends_with?(".#{website_host}") | |
| end | |
| def both_have_subdomains?(website_host, discourse_host) | |
| website_host.split('.').length == discourse_host.split('.').length && | |
| discourse_host.split('.').length > 2 | |
| end |
🧰 Tools
🪛 RuboCop (1.76.1)
[convention] 137-151: Assignment Branch Condition size for website_name is too high. [<2, 35, 12> 37.05/23]
(Metrics/AbcSize)
[convention] 137-151: Cyclomatic complexity for website_name is too high. [8/7]
(Metrics/CyclomaticComplexity)
[convention] 137-151: Perceived complexity for website_name is too high. [9/8]
(Metrics/PerceivedComplexity)
[convention] 138-138: Avoid using rescue in its modifier form.
(Style/RescueModifier)
🤖 Prompt for AI Agents
In app/serializers/user_serializer.rb around lines 137 to 151, the website_name
method is too complex and hard to read due to multiple nested conditionals and
repeated URI parsing. Refactor by extracting URI parsing into a separate method,
and split the conditional logic into smaller helper methods that each handle one
comparison case. This will reduce cyclomatic complexity and improve readability
by making each method focused and easier to understand.
2 participants
Test 6
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests